Skip to content

feat(daemon-cli): mcpdo — experimental connection CLI client (#1432) - #1783

Open
BobDickinson wants to merge 35 commits into
v2/mainfrom
v2/mcpi-client
Open

BobDickinson wants to merge 35 commits into
v2/mainfrom
v2/mcpi-client

Conversation

@BobDickinson

@BobDickinson BobDickinson commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1432

Summary

Adds clients/daemon-cli, an experimental connection CLI published as the mcpdo bin: connect to an MCP server once, then run many commands against that named connection (ssh-agent style). Connections are held by an implicit local Unix-socket daemon that mcpdo starts on demand and talks to over token-authenticated NDJSON.

mcpdo connect test-stdio --config path/to/mcp.json
mcpdo tools/list
mcpdo tools/call echo message:=hi
mcpdo --conn other tools/list      # or --connection
mcpdo logging/tail                 # long-lived stream; Ctrl-C to stop
mcpdo connections/list && mcpdo daemon status
eval "$(mcpdo private)"            # optional per-shell private daemon
  • Daemon security model: per-daemon bearer token always required (generated at startup, published 0600 as daemon.token, or supplied via env for private mode), 0700 socket dirs under $TMPDIR/mcp-conn-<uid>/, socket-path length validated up front, O_EXCL pid lock with dead-pid reclaim (no takeover of a live daemon), 1 MiB NDJSON request-line cap, daemon stderr to a 0600 daemon.log.
  • Output safety: terminal-bound text (results, elicitation prompts, daemon errors) is control-character sanitized; OSC 8 URIs validated; --format json stays verbatim.
  • Stdio correctness: connect always sends an absolute cwd (defaults to the caller's), bare command names are resolved against the caller's PATH client-side, and the daemon chdirs away from its spawn directory.
  • Auth: shared oauth.json with the other Inspector clients; connect-time OAuth on this CLI (--relogin, --stored-auth-only); elicitation bridging for form/URL prompts (--format json auto-declines forms; URL mode never auto-accepts).
  • Era support: negotiates legacy/modern via core InspectorClient; --era legacy|auto|modern on connect.
  • Reuses clients/cli handlers / error-handler / OAuth helpers via a temporary build-time @inspector/cli alias — chore(mcpi): replace the temporary @inspector/cli source alias with a real shared surface #2461 tracks promoting that surface to a shared area.
  • Wired into monorepo validate / build / coverage / verify:bundle-externals; documented in AGENTS.md, clients/daemon-cli/README.md, and specification/v2_cli_v2.md. Adds a top-level skills/mcpdo end-user skill (teaches an agent to drive mcpdo), distinct from the .claude/skills/ repo procedures.

Packaging

Published by this PR (maintainer-approved): root bin.mcpdo → clients/daemon-cli/build/mcp-bin.js; files adds clients/daemon-cli/build and skills/mcpdo. Adds ~199 KB compressed (~770 KB unpacked, 16%) to the tarball. The daemon is inert unless mcpdo is invoked. The bin was renamed from mcpi to mcpdo to avoid the existing unrelated mcpi npm package.

Naming

Reviewer-visible rename since the last review: the client moved from clients/mcpi to clients/daemon-cli, the bin is mcpdo, and user-facing vocabulary moved from "session" to "connection" (connections/list|show|use, --connection/--conn) — modern MCP is session-less and the daemon-held thing's lifecycle is the connection's.

Test plan

  • npm run coverage:daemon-cli — 259 tests, per-file coverage gate ≥90 on all four dimensions (only the two true bootstraps src/mcp-bin.ts / src/daemon/run.ts excluded)
  • npm run verify:bundle-externals (daemon-cli enrolled, 4 bundles)
  • npm run local:gate from repo root — green on macOS
  • Manual: connect/tools/resources/logging-tail against test-servers over stdio + HTTP, OAuth + EMA connects, shared and mcpdo private daemons

@BobDickinson BobDickinson added the v2 Issues and PRs for v2 label Jul 25, 2026
Base automatically changed from v2/cli-improvements to v2/main July 26, 2026 20:55
@cliffhall cliffhall linked an issue Aug 17, 2026 that may be closed by this pull request
BobDickinson added a commit that referenced this pull request Sep 14, 2026
Adds a per-connection override for the elicitation capability mcpi
advertises to a server, mirroring the existing --era mechanism:

- InspectorServerSettings.elicitCapability ("off"|"url"|"form"|"both",
  default "both") persists on disk as elicitCapability, omitted when it
  equals the default, and round-trips through serverList.ts the same
  way protocolEra does.
- mcpi connect gains --elicit <mode>, validated the same way as --era,
  with a withElicitOverride() helper mirroring withEraOverride() (incl.
  synthesizing bare-defaults settings for ad-hoc targets).
- createSessionClient() now derives the InspectorClient elicit option
  from serverSettings.elicitCapability via elicitCapabilityToClientOption()
  instead of the old Phase-1 hardcoded { url: true, form: true }.

This lets a caller that cannot handle an interactive elicitation prompt
(a script, an agent) opt out entirely so the server sees no elicitation
capability and can fall back to its own alternative, instead of every
elicitation request being auto-declined.

Also updates clients/mcpi/README.md with an "Elicitation support"
section (previously undocumented, despite already-shipped URL/form
prompt rendering) and the --elicit flag, and refreshes the stale
"Sampling / elicitation CLI: Still TUI/web" to-do row in
specification/v2_cli_v2.md.

Manually verified end-to-end against the modern-mrtr-http test server:
--elicit off makes the server itself reject the mid-round input request
("capabilities do not declare the required capability"); --elicit both
(default) succeeds and reaches the interactive/auto-decline prompt path
as before.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

mcpi work summary — PR #1783 (2026-09-13 → 2026-09-14)

Branch: v2/mcpi-client in /Users/bob/Documents/GitHub/inspector-trees/v2-mcpi-client
Repo: modelcontextprotocol/inspector
PR: #1783 — all pushed; CI (build, coverage) green as of 7fd4af59.

Organized by what actually changed functionally, not commit order.


1. Modern protocol-era support (era + skills primitives)

Gave mcpi first-class awareness of MCP's protocol eras (legacy vs. modern/
task-capable) and filled in missing skills primitives:

  • --era override on connect (f2fc1a2c): force which protocol era an
    ad-hoc session negotiates as, instead of only auto-detecting.
  • sessions/show replaces initialize (9550b32c): the session-info RPC
    now reports era details directly (protocol version, task support, etc.)
    instead of the old bare initialize response.
  • protocolEra surfaced everywhere (380dd3e7): every session listing
    (sessions/list, not just sessions/show) now reports era at a glance.
  • tasks/update (22d5c9f4): implemented to resume paused "modern"
    (task-capable) MCP tasks, with success/error-path tests.
  • skills/list and skills/get (40b4f441): implemented the RPCs
    (previously stubbed/missing), supporting positional and --uri argument
    forms, plus a --verify flag on skills/list.
  • Docs (2a61f427): documented --era, sessions/show, and
    tasks/update end-to-end.

2. Elicitation features (legacy URL-mode and modern/MRTR form-mode)

Built out MCP's elicitation flow, covering both eras' mechanisms:

  • URL-mode (legacy elicitation) (ac7e4bf1): when a server elicits via a
    URL, mcpi prompts to confirm/open it and waits for completion.
  • Form-mode (modern/MRTR structured elicitation) (258d789f): when a
    server elicits structured form data (JSON-schema-driven, per the newer
    request-response/MRTR-style pattern), mcpi walks the user through each
    field interactively with a review step before submitting.
  • --elicit capability override (98a41510): lets a caller declare
    elicitation support explicitly, for ad-hoc/non-standard clients.

3. Making mcpi agent-friendly

Everything else — reframing and hardening mcpi so an AI agent driving it
non-interactively gets the same guarantees a human at a terminal gets:

  • Packaging (29193232): bundled mcpi into the published
    @modelcontextprotocol/inspector npm package so it actually ships.
  • mcpi agent-help + skills/mcpi/SKILL.md (9ecd2647): a discoverable,
    self-contained reference for agents on how to drive mcpi non-interactively.
  • OAuth without a TTY (0096e2c7): OAuth's URL-prompt-and-wait flow no
    longer requires an interactive terminal; message reframed for an
    agent-attended flow ("The user needs to navigate to this link to
    authenticate: <url>"). Added clean SIGINT/SIGTERM cancellation so a user
    (or agent) can break out of the ~15-minute OAuth wait if they decide not to
    auth or auth fails, instead of it being a hard, uninterruptible block. Also
    addressed the daemon idle-timeout interacting with long OAuth waits.
    Live-tested with a real, non-TTY OAuth flow.
  • Non-TTY elicitation (7cf45384): removed the TTY gate on elicitation
    entirely — both URL-mode and form-mode now work non-interactively, since
    the underlying readline-based prompting was never actually TTY-dependent,
    just gated by policy. Closed the one real risk this exposed (stdin EOF/close
    could hang readline.question() forever) by racing every prompt against a
    "stdin closed" signal. Live end-to-end tested against a real MCP test
    server, including a piped-EOF instant-decline case and a live-FIFO
    simulated-agent-relayed-answer case.
  • Final non-TTY audit + SIGINT cleanup (7fd4af59): audited all
    remaining isTTY gates; confirmed auth/clear --all and
    requireExplicitSession()'s explicit-session requirement are intentional
    (see MRU note below), fixed a stale doc comment, and extended clean
    SIGINT/SIGTERM cancellation from the two streaming RPCs to the general
    rpc path so Ctrl-C during any blocking call (e.g. tools/call, an
    elicitation wait) cancels cleanly instead of killing the process.

Key design note (MRU): the daemon is a single shared process, so MRU
("most recently used" session) state is global, not per-terminal.
requireExplicitSession() gates on stdin, not stdout, so a human piping
output (mcpi tools/list | jq) still gets MRU convenience; a truly
non-interactive caller (agent/script/CI) must pass --session/@name
explicitly, since there's no live human to catch a wrong guess.
MCP_ALLOW_DEFAULT_SESSION=1 opts back into MRU for scripts that want it.


Non-functional maintenance (excluded from the above as "not changes")

These kept the branch buildable/green but didn't change behavior:

  • e79dea7f, fd64afff — restored build:dev tooling/build config after a
    v2/main merge broke it.
  • 54d00b91 — brought mcpi's validate scripts into parity with the rest of
    the repo's guards.
  • aabc19fa, 12353bea — closed CI coverage/build gaps (including one
    caused by the agent-help commit itself shipping without tests) — pure
    test-coverage backfill, no functional change.

@cliffhall cliffhall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mergeability verdict: ❌ Not mergeable — changes requested

CI is green and the feature works end to end. I connected, listed and called tools, and checked sessions and daemon status against a stdio test server. The test suite is large and mostly good. Four things block the merge:

  1. Private-mode daemons can be taken over, and live sessions orphaned (security, reproduced).
  2. Server-controlled terminal escape sequences reach the user's terminal raw (security, reproduced).
  3. Stdio connect resolves relative commands against the daemon's cwd, not the caller's, so it can run a different file than the one the user named (reproduced).
  4. Repo-rule violations: dependency placement, coverage-gate exclusions, no DCO signoff on any of the 30 commits, and a test that fails on stock macOS so local:gate cannot go green on a Mac.

There is also a scope question maintainers need to decide explicitly, not by drift: this PR now publishes a global mcpi bin and a background daemon to every installer of @modelcontextprotocol/inspector, while the PR description still says it does not.

Everything below was checked in a clean worktree of 22c97b09 (npm install && npm run build, then validate:guards, coverage:mcpi and verify:bundle-externals) on macOS 15, with an isolated MCP_STORAGE_DIR / MCP_INSPECTOR_DAEMON_DIR.


1. Security

Most of the risk comes from what mcpi adds on top of the one-shot CLI: a detached, long-lived process that accepts connect requests carrying an arbitrary serverConfig (including a stdio command) over a Unix socket, and then spawns that command. The one-shot CLI's exposure ends when the process exits. The daemon's does not: it stays up for as long as any session is open, because the idle timer only arms at zero sessions.

The stated trust model is same-UID filesystem trust (shared mode), plus an IPC token in private mode. The findings below are measured against that model.

1a. 🔴 A wrong or missing token replaces a live private daemon (blocker)

ensureDaemon() (clients/mcpi/src/daemon/ensure.ts) treats "socket reachable but ping failed" as "stale socket". It unlinks the socket and spawns a new daemon. ping also fails on daemon_auth_failed, so any caller holding the wrong token, or no token, deletes the live daemon's socket and installs its own daemon in its place.

Reproduced:

# user starts a private daemon
MCP_INSPECTOR_DAEMON_TOKEN=goodtoken mcpi connect --session a node server.js   → daemon 90433

# any same-UID process without the token
mcpi connect --session evil node server.js                                      → spawns daemon 90441 (NO token)

# the legitimate user, still presenting goodtoken
MCP_INSPECTOR_DAEMON_TOKEN=goodtoken mcpi sessions/list
Sessions (1):
* `@evil` (MRU) — node server.js [legacy]

Three consequences:

  • Private mode's guarantee is void. The user's token-bearing client is silently served by an unauthenticated daemon that someone else started. The replacement's @evil session is now the user's MRU default, so the user's next bare mcpi tools/call … goes to a server the other party chose. The token exists to separate same-UID callers; this is the one thing it currently fails to do.
  • Orphaned processes. The original daemon, 90433 above, and its stdio server keep running with no reachable socket. They never exit, because idle reaping is only armed at zero sessions. The same happens with a wrong token (MCP_INSPECTOR_DAEMON_TOKEN=wrong also produced a second daemon). A typo therefore leaks processes.
  • Lock-out. With a wrong token, the legitimate user then gets daemon_auth_failed against the replacement.

Fix: on daemon_auth_failed (or any structured error reply), ensureDaemon must fail loudly and leave the socket alone. Only a socket that refuses connections (ECONNREFUSED / ENOENT) is stale. daemon.lock is written but never used as a lock. Make it one: store pid plus start time, use an O_EXCL create or proper-lockfile, which is already a root dependency, and check liveness with process.kill(pid, 0) before unlinking anything.

1b. 🔴 Terminal escape injection from server-controlled text (blocker)

The human formatter (clients/mcpi/src/session/format-human.ts, the default --format text) writes server-supplied strings straight to the terminal: tool results, descriptions, resource text, elicitation messages, and URIs embedded in OSC 8 hyperlinks (clients/cli/src/style.ts). I found no control-character stripping anywhere under clients/mcpi/src.

Reproduced with the echo test tool (od -c of stdout):

E c h o :   h i 033 ] 5 2 ; c ; c H d u Z W Q = \a 033 ] 0 ; S P O O F E D - T I T L E \a

That is an OSC 52 clipboard write and an OSC 0 title change, both delivered intact from the server. Depending on the terminal, the same channel allows clipboard poisoning (the next paste into a shell), hiding or overwriting earlier output (CSI cursor moves and erases), and spoofing link targets. A URI containing \a also breaks out of the OSC 8 wrapper. The one-shot CLI emits JSON, where these bytes are escaped, so this is new exposure introduced by this PR. It matters more because the skill in this PR targets agents reading the output.

Fix: sanitize every server-derived string before styling it. Replace C0/C1 controls other than \n and \t, plus DEL, with visible escapes such as \x1b → ␛ or \u001b. Validate or percent-encode URIs before putting them in OSC 8. --format json is already safe.

1c. 🟠 Stdio commands resolve against the daemon's cwd, not the caller's

The daemon is spawned without cwd, so it inherits the directory of whichever mcpi invocation first started it. connect does not default --cwd to the caller's process.cwd() (clients/mcpi/src/session/mcp.ts, serverOptions.cwd). A relative stdio target is therefore resolved in someone else's directory:

(daemon started from the repo root; lsof cwd → /…/mcp-inspector-pr1783)
cd test-servers/build && mcpi connect --session rel node ./test-server-stdio.js
{"error":{"code":"error","message":"Connection closed"}}

Here it failed with an opaque error, but only because the file did not exist in the daemon's cwd. If a file with the same name exists there, mcpi silently runs that one. mcpi connect node ./server.js in project B would execute project A's ./server.js. That is a correctness bug with a real security edge. PATH has the same staleness problem: every later session inherits the first shell's PATH, whether that came from nvm, a venv or anything else.

Fix: the front end should always send an absolute cwd, defaulting to process.cwd(), and should resolve relative command paths before sending. It should also consider forwarding the caller's PATH. The daemon should chdir to its own directory, or to /, at startup so it never pins an arbitrary working directory.

1d. 🟠 The daemon dies silently, and the socket-path limit is unchecked

ensure.ts spawns the daemon with stdio: "ignore", so every startup failure is invisible. The client waits 10s and reports a generic daemon_start_timeout. I hit this at once: my scratch MCP_INSPECTOR_DAEMON_DIR produced a 140-byte socket path, and listen() fails above macOS's 104-byte sun_path limit (108 on Linux). The daemon exited, left a stale daemon.lock behind (mode 0644, because the chmod never ran), and the user saw only a timeout.

The private-mode layout, $HOME/.mcp-inspector/private/<uuid>/daemon.sock, uses 72 fixed bytes, leaving ~32 bytes for $HOME on macOS. This is also why a test fails locally (see §2c).

Fix: validate the socket path length up front with a clear error, and shorten the private layout, e.g. a short id, or $TMPDIR/mcpi-<uid>/<short> with a 0700 dir. Send the daemon's stderr to daemon.log in the daemon dir with mode 0600, and have daemon_start_timeout include its tail.

1e. 🟡 Hardening (not blocking on their own)

  • Shared-mode directory permissions. ensureDaemonDir() creates ~/.mcp-inspector with the default umask (0755 here). The socket is chmod 0600 only after listen(), which leaves a short window. Create the dir 0700 and bind inside it. Then the socket's own mode never matters, and BSD's inconsistent enforcement of socket permissions stops mattering too.
  • An exec service outside agent sandboxes. This deserves a paragraph because the PR ships a skill aimed at agents. In shared mode, any same-UID process that can connect() to ~/.mcp-inspector/daemon.sock can have the daemon spawn any command. Same-UID is nominally the same privilege, but agent sandboxes (Claude Code's sandbox, Codex, containers that bind-mount $HOME, Flatpak) often restrict exec and filesystem access and not Unix-socket connects. A daemon started outside a sandbox, by the human, becomes an unsandboxed exec endpoint for any agent inside one. My suggestion: always require a token, including in shared mode, stored in a 0600 file in the 0700 daemon dir. That gives no extra protection against a plain same-UID process, which can read the file anyway. It does give sandbox policies a file-read denial to rely on, which is the control they actually have. It also retires the tokenless path that 1a exploits.
  • No request-size limit on the NDJSON reader (readline over the socket). A single unbounded line grows daemon memory without limit. Cap the line length.
  • Non-TTY elicitation answered by an agent (7cf4538). This is a real product decision, not a bug. Form-mode elicitation is meant to put a question to the user, and this makes it routine for an agent to answer on the user's behalf. That may be fine for an inspector, but please record it as a decision (spec doc plus README), and keep URL-mode elicitation requiring an explicit human action. skills/mcpi/SKILL.md also still says "running non-interactively (no TTY, scripted, or --format json) auto-declines", which that commit made false. Only --format json declines now.
  • core/auth/node/runner-interactive-oauth.ts now installs process-wide SIGINT/SIGTERM listeners for the length of the OAuth wait. They are correctly removed in finally, but this is shared core/ and also runs under the TUI, which owns Ctrl-C through Ink. Please confirm that TUI Ctrl-C during an OAuth wait still behaves as intended, or scope the handler to callers that opt in.

Good things worth keeping: timingSafeEqual token comparison, 0600 socket and lock, 0700 private dirs, randomBytes(32) tokens, the POSIX-safe single-quoting in mcpi private, idle self-reaping, and stdio children exiting when their stdin closes. I SIGKILLed the daemon, and its test server exited with no orphans.


2. Repo-rule compliance (AGENTS.md)

2a. 🔴 Dependency placement

  • clients/mcpi/package.json re-declares root-owned runtime dependencies: @modelcontextprotocol/{client,core,server,server-legacy}, ajv, atomically, @napi-rs/keyring, pino, undici, zod, commander and open. This breaks "a client declares only what that client alone consumes … clients/cli and clients/launcher therefore declare no runtime dependencies". It re-creates exactly the second copy that #1896 exists to prevent (the 3,387-line clients/mcpi/package-lock.json). server/server-legacy are not runtime dependencies of a client at all.
  • The re-declaration is also load-bearing, which is why this matters beyond neatness. tsup auto-externalizes what the client's manifest declares, and mcpi's external list omits undici, zod, ajv and atomically. Deleting the manifest entries today would inline undici and reproduce #2067 (Dynamic require of "assert" is not supported). Fix: delete the client runtime deps and name every root runtime dependency that core/ reaches in clients/mcpi/tsup.config.ts external, mirroring clients/cli/tsup.config.ts and its comments.
  • AGENTS.md still says "must also be named in all three bundler external lists (clients/{cli,tui}/tsup.config.ts, clients/web/tsup.runner.config.ts)". With a fourth bundler this rule changes, so update AGENTS.md in the same change, per the maintenance rule.

2b. 🔴 Coverage gate: whole files waved out

clients/mcpi/vitest.config.ts excludes src/daemon/ipc-glue.ts and src/daemon/stream-client.ts from the ≥90 per-file gate as "hard-to-stabilize accept/stream races". The rule is explicit: "A genuinely-unreachable branch is annotated at the source, never waved through by lowering the gate", and a race is fixed with an awaited condition, never with headroom or exclusion (#1596). ipc-glue.ts is the socket accept loop and the elicitation line-consumer, the most security-relevant code in the client. It is the file that most needs gating. Only the true bootstraps (mcp-bin.ts, daemon/run.ts) qualify for exclusion, as with clients/cli's src/index.ts. The AGENTS.md edit that documents these exclusions should be dropped along with them.

2c. 🔴 A test fails locally, so local:gate cannot pass on macOS

coverage:mcpi → daemon-private.test.ts > ensureDaemon spawns a token-gated daemon from env fails on stock macOS: 1 failed | 220 passed, with Timed out waiting for session daemon. The test sets HOME to os.tmpdir()/…, which on macOS is /var/folders/…/T/. The resulting socket path is 142 bytes (§1d). CI passes only because Linux's /tmp is short. local:gate is the mandatory pre-push command, and the PR's own test plan still has npm run ci unchecked. Fixing §1d fixes this too.

2d. 🔴 DCO

None of the 30 commits carries Signed-off-by (git log --format='%(trailers:key=Signed-off-by)' is empty for every one). AGENTS.md: "sign off every commit (git commit -s — the DCO check is a hard merge gate with no partial credit)". A rebase with --signoff fixes it. It is probably best done together with the rebase in §2f.

2e. 🟠 Docs and structure

  • The PR description is stale and contradicts the code. It says "Not in the published tarball (files allowlist unchanged)" and "No root bin.mcpi". 2919323 adds "mcpi": "./clients/mcpi/build/mcp-bin.js" to root bin, and adds clients/mcpi/build and skills/mcpi to files. It also still says "Depends on #1782" (merged) and "Retarget … after #1782 merges". Please rewrite it; reviewers and the release notes will read it.
  • The new top-level skills/ directory is missing from the AGENTS.md / README Project Structure trees. Its relationship to .claude/skills/ needs one line (end-user skill shipped in the tarball vs. repo procedures). Otherwise the next agent will try to run verify:skills rules against it, or move it.
  • Branch name v2/mcpi-client lacks the type/issue segment (v2/feat/1432-mcpi-client). This is minor and not worth a new branch now.

2f. 🟠 Freshness and size

The branch is 124 commits behind v2/main. That includes #2374's Skills registry and -32021 changes and the SDK-v2 client-extension work, which touch the same skills/list / skills/get / era surfaces mcpi wraps. mergeStateStatus says CLEAN, but green CI on a stale base proves little for this surface. Please rebase with --signoff and re-run local:gate. At 16.8k added lines, with two merge commits and features accreted over two months (elicitation, EMA, tasks/update, era, packaging), this is hard to review as a whole. At minimum, the packaging change (2919323) should be split into its own PR so it gets its own decision (see §3).

2g. 🟡 Architecture: the @inspector/cli reach-in

The build-time alias from clients/mcpi into clients/cli/src (handlers, error-handler, OAuth navigation) makes one client's private source another client's API. clients/cli refactors can now break mcpi with no signal in the cli's own gate. fd64aff and aabc19f are both exactly this kind of breakage. The code comments say "temporary"; please file the tracking issue now (move handlers/, error-handler, and cli-oauth-navigation into core/, or a shared Node-runner area) and link it from the tsup comment. Temporary without an issue tends to become permanent.


3. Should it ship in the published package? Should it be containerized?

Shipping. Publishing adds a second global bin and a long-lived background daemon to every npm i -g @modelcontextprotocol/inspector install, under a name maintainers haven't signed off on. (mcpi is also an existing, unrelated npm package, a Minecraft-Pi API, which is harmless for a bin but will confuse search and npx mcpi.) The issue and spec still call this experimental. I'd keep it out of the tarball until the security items above are fixed and a maintainer signs off on the bin name, then publish it in a dedicated PR. That was the original plan in this PR's description, and I think it was right.

Containerizing the daemon: should not, and mostly could not usefully. The daemon's whole job needs host resources: the OS keychain (@napi-rs/keyring), the shared oauth.json store, the user's browser for OAuth, a loopback OAuth callback port, and above all local stdio servers that exist to touch the user's files and tools. Putting the daemon in a container breaks keyring and OAuth, turns every stdio server into a mount-and-PATH configuration problem, and on macOS and Windows adds a Linux VM dependency (Docker Desktop, Podman). It also secures the wrong thing. The daemon itself is small, trusted first-party code. The risky parts are (a) the socket as an exec endpoint, which §1a and §1e fix in code, and (b) the MCP servers it runs, which are untrusted third-party code. That is the same risk every MCP host takes, and containers are the right tool for it.

What I'd recommend instead:

  1. Fix the socket boundary in code (§1a, §1e). That is the risk the daemon adds.
  2. Make server isolation opt-in, per session. Document the recipe that already works today with no code: mcpi connect docker run -i --rm --network none -v "$PWD:/work:ro" <image>. Then consider a first-class --sandbox on connect that wraps the stdio command: docker/podman run -i everywhere, with lighter native options later (bwrap on Linux, sandbox-exec profiles on macOS). That puts isolation where the untrusted code is, lets users choose it per server, and costs nothing when unused.
  3. Treat HTTP/SSE targets as needing no process isolation. Their risk is the terminal-output and elicitation surface (§1b, §1e), which sanitization covers.

Summary of requested changes

# Change Severity
1a ensureDaemon: never unlink or replace on auth failure; turn daemon.lock into a real pid lock 🔴 security
1b Sanitize control characters in all server-derived text output; validate OSC 8 URIs 🔴 security
1c Send an absolute cwd (default process.cwd()) with stdio connects; chdir the daemon away 🟠 security/correctness
1d Socket-path length check, shorter private layout, daemon stderr to a 0600 log 🟠
1e 0700 daemon dir; token always required (file-backed); cap NDJSON line length; fix SKILL.md elicitation wording; confirm TUI Ctrl-C 🟡
2a Drop client runtime deps; complete the external list; update AGENTS.md's "three lists" rule 🔴 rules
2b Gate ipc-glue.ts / stream-client.ts (fix races, v8 ignore only truly unreachable lines) 🔴 rules
2c Make daemon-private.test.ts pass on macOS (falls out of 1d) 🔴 rules
2d --signoff every commit 🔴 rules
2e/2f Rebase on v2/main, rewrite the PR description, document skills/, split out packaging 🟠
2g File the tracking issue for the @inspector/cli reach-in 🟡

Happy to re-review once the 🔴 items are in. The session model itself works well and I'd like to see it land.

BobDickinson and others added 2 commits September 22, 2026 22:25
Small, mcpi-motivated additions to shared code, kept separate so the
client itself is reviewable on its own:

- clients/cli handlers: expose method metadata (method-types) and a
  reusable run-method entry point for out-of-process callers; unit
  tests for the mocked run-method paths
- clients/cli/src/cli-oauth-navigation.ts: allow callers to supply
  their own browser-open/navigation hooks
- core/auth/node/runner-interactive-oauth.ts: SIGINT/SIGTERM-aware
  wait so Ctrl-C during an interactive OAuth flow cleans up the
  callback server (removed in finally); test in clients/web test tree
- core/mcp/serverList.ts, core/mcp/types.ts: server-list helpers and
  types shared by cli and mcpi

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Add clients/mcpi, an experimental session-oriented CLI: connect once,
then run many MCP commands against a named session held open by an
implicit local Unix-socket daemon (ssh-agent style). Not part of the
published package; runs from a repo checkout (npm link).

Highlights:

- Session daemon (auto-spawned, idle self-reaping) with NDJSON IPC,
  token-gated private mode (`mcpi private`), MRU session selection
- Full command surface via shared clients/cli handlers: tools,
  resources, prompts, skills, tasks, completions, logging, sampling,
  elicitation (interactive form prompts and agent-answerable modes)
- OAuth support including stored-token reuse, interactive browser
  flows, and enterprise-managed auth (EMA): --ema connect flag,
  auth/ema-status|login|logout, per-session Auth reporting with
  disk-truth reads in sessions/show
- Era detection/reporting (legacy vs 2025-11-25) per session
- Human and JSON output formats; agent-focused skills/mcpi/SKILL.md
- Spec: specification/v2_cli_v2.md; docs in clients/mcpi/README.md
- Tests: 221 unit/integration tests, per-file coverage gates wired
  into the repo quality gate (coverage:mcpi, validate:mcpi)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
BobDickinson and others added 7 commits September 23, 2026 11:03
1a — no daemon takeover: a socket that accepts connections is owned by a
live daemon; any ping failure (auth, timeout, protocol) now fails loudly
instead of unlinking the socket and respawning over it. daemon.lock is a
real O_EXCL pid lock with dead-pid reclaim, closing the probe/unlink/bind
race between two starting daemons.

1b — terminal escape sanitization: every server-controlled string is
sanitized before reaching the terminal in text mode (new
session/sanitize.ts: C0/C1 controls except \n\t become visible
stand-ins). Wired into the human formatter, the ndjson stderr summary,
elicitation prompts (message/url/schema — never protocol ids), and
daemon-client error messages. --format json stays verbatim (JSON already
escapes controls).

1c — stdio cwd correctness: --cwd is resolved to an absolute path at the
caller; stdio connects with no cwd default to the client's cwd
(catalog/--cwd still win); the daemon chdirs to its own dir on startup so
its inherited cwd is inert.

1d — no silent daemon death: socket paths are validated against sun_path
limits up front with an actionable error; private daemon dirs moved to
the short $TMPDIR/mcpi-<uid>/<id>/ layout (0700, fits the macOS limit);
daemon stderr goes to a 0600 daemon.log whose tail is quoted in
start-timeout errors.

1e — hardening: daemon dir created 0700; a token is now always required —
generated when the environment doesn't supply one and published to a
0600 daemon.token beside the socket for clients to read, retiring the
unauthenticated request path; NDJSON request lines are capped at 1 MiB;
SKILL.md/README/spec updated to record the elicitation decision (only
--format json auto-declines; URL mode never auto-accepts); the OAuth
runner's process-wide SIGINT/SIGTERM handlers are now opt-in
(handleSignals) so the TUI keeps Ctrl-C ownership under Ink, with CLI and
mcpi opting in.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
clients/mcpi declared root-owned runtime dependencies, re-creating the
second copy the dependency-placement rule (#1896) exists to prevent, and
the re-declaration was load-bearing: tsup auto-externalized from the
client manifest, so the external list was incomplete.

- clients/mcpi/package.json now declares no runtime dependencies (same
  steady state as clients/cli and clients/launcher); the 3,387-line
  lockfile shrinks to devDeps only.
- clients/mcpi/tsup.config.ts names every root runtime dependency that
  core/ (or the bundled one-shot CLI source) reaches, mirroring
  clients/cli/tsup.config.ts; verify:bundle-externals passes against the
  built output.
- A scoped override pins sucrase's nested commander to ^13: with no
  top-level commander declared, npm otherwise hoists sucrase's
  commander@4 into clients/mcpi/node_modules where it shadows the root
  commander@13 on the walk-up (helpCommand crash at startup).
- AGENTS.md's "three lists" rule is now four (clients/{cli,mcpi,tui}
  tsup configs + web's runner config); the no-runtime-deps steady state
  names mcpi; sdk-watch's checklist string updated to match.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…review 2b)

Remove the "hard-to-stabilize accept/stream races" coverage exclusions
for src/daemon/ipc-glue.ts and src/daemon/stream-client.ts; only true
bootstraps (src/mcp-bin.ts, src/daemon/run.ts) stay outside the gate.

New __tests__/daemon-ipc-glue.test.ts exercises the per-connection
wiring deterministically with an in-memory Duplex (no accept races):
the elicitation channel round trip, non-answer lines, double-pending
rejection, disconnect/destroyed-socket rejection, the mid-handle
destroyed guard, and single-shot stream cleanup on socket error.
daemon-stream.test.ts gains default socket-path/timeout + explicit
token coverage and a post-end frame-ignore case.

Writing those tests surfaced a real bug: readline re-emits socket
errors on the interface, so a client RST would have crashed the daemon
with an unhandled 'error' event. acceptDaemonConnection now attaches an
rl error listener; the socket error handler keeps owning teardown.

Both files clear >=90 on all four dimensions (ipc-glue 99/95/95/100,
stream-client 96/92/93/97); mcpi suite 250/250.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…es (review 2e, 2g)

AGENTS.md and README gain the skills/ entry in the project tree
(distinct from .claude/skills/); the temporary @inspector/cli alias
notes in AGENTS.md, clients/mcpi/README.md and tsup.config.ts now link
the tracking issue #2461.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dio servers (review §3)

The daemon token gates who can command the daemon, not what a spawned
server can do. Record the zero-code recipe — wrapping the stdio command
in `docker run -i` — as the way to isolate an untrusted server, per the
review recommendation.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… (review 1c follow-up)

The daemon inherits the environment of whichever mcpi invocation first
spawned it, so a bare command name like `node` was looked up in that
stale PATH — a different nvm version or venv could supply a different
binary than the caller's shell would. The connect front end now
resolves bare names (no path separator) to an absolute path using the
caller's PATH before the config crosses the IPC boundary, so the
daemon spawns exactly the caller's binary and no environment is
forwarded. Unresolvable names pass through unchanged so the daemon's
spawn error stays the user-visible failure; commands with a separator
still resolve against the pinned session cwd.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…bundle into the package

Maintainer-approved decisions on the #1783 review thread:

- Bin name: `mcpdo` (conflict-free on npm; `mcpi` collides with an
  unrelated package). Root `bin` now installs it and `files` ships
  `clients/daemon-cli/build` and `skills/mcpdo`, so
  `npm i -g @modelcontextprotocol/inspector` provides the experimental
  client (~200 KB compressed addition).
- Internal name: `clients/daemon-cli` (role-based, like cli/tui/web/
  launcher), insulated from future bin renames. Root scripts are now
  build:/validate:/coverage:daemon-cli.
- Vocabulary: the daemon holds named live connections, not resumable
  sessions, so the session wording over-promised and collided with MCP
  transport terminology. Commands are now `connections/list|show|use`;
  `connect`/`disconnect` stay top-level lifecycle verbs. The global flag
  is `--connection <name>` with `--conn` as a documented shorthand
  (argv-level alias, one option registration). Env opt-in renamed to
  MCP_ALLOW_DEFAULT_CONNECTION; daemon dirs move to
  $TMPDIR/mcp-conn-<uid>/. IdP *session* wording is kept where it names
  the enterprise IdP login session (a different concept).
- Shared cli helpers consumed only by mcpdo follow suit
  (annotateServerEntriesWithConnections, CONNECTION_RPC_METHODS, and the
  servers/list `connection` annotation field).

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson BobDickinson changed the title feat(mcpi): experimental session CLI client (#1432) feat(daemon-cli): mcpdo — experimental connection CLI client (#1432) Sep 23, 2026
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — reproducing 1a–1c made these easy to fix with confidence. Everything below is on the branch; local:gate is green on macOS (the §2c failure is gone). Two headline changes since your review, both maintainer-approved: the client is renamed (mcpi → mcpdo, clients/mcpi → clients/daemon-cli, "session" → "connection" vocabulary), and packaging is folded back into this PR (details under §3).

§1 Security

  • 1a — fixed. ensureDaemon now fails loudly on any structured error reply (including daemon_auth_failed) and never unlinks the socket; only ECONNREFUSED/ENOENT is treated as stale. daemon.lock is a real O_EXCL pid+starttime lock with a kill(pid, 0) liveness check before any reclaim. Your repro sequence now errors instead of replacing the daemon.
  • 1b — fixed. All server-derived terminal-bound text goes through a sanitizer (C0/C1 + DEL → visible escapes, \n/\t preserved); OSC 8 URIs are validated before wrapping. Covered by tests including your OSC 52/OSC 0 payloads. --format json unchanged.
  • 1c — fixed. connect always sends an absolute cwd, defaulting to the caller's process.cwd(); the daemon chdirs to its own directory at startup. For PATH we went a step further than forwarding: bare command names are resolved client-side against the caller's PATH (which-style) and sent absolute, so no environment crosses the socket at all.
  • 1d — fixed. Socket path length is validated up front against the platform sun_path limit with a clear error; the layout is shortened to $TMPDIR/mcp-conn-<uid>/<id>/ (0700); daemon stderr goes to a 0600 daemon.log and start-timeout errors include its tail.
  • 1e — all taken. Daemon dir created 0700 before bind; token always required in every mode (0600 daemon.token in the 0700 dir — adopted your reasoning: it gives sandbox policies a file-read denial to enforce and retires the tokenless path from 1a); 1 MiB NDJSON request-line cap; SKILL.md elicitation wording corrected and the non-TTY-agent-may-answer decision is recorded in specification/v2_cli_v2.md (URL mode still requires an explicit answer); the OAuth SIGINT/SIGTERM handlers were a regression introduced by this PR's own first commit — they're now opt-in (handleSignals), so TUI Ctrl-C behavior is unchanged.

§2 Repo rules

  • 2a — fixed. Client runtime deps removed; every root runtime dependency core/ reaches is named in clients/daemon-cli/tsup.config.ts external, mirroring clients/cli; AGENTS.md's "three lists" rule updated to four.
  • 2b — fixed. ipc-glue.ts and stream-client.ts are in the ≥90 per-file gate; the accept/stream races were fixed with awaited conditions, and only genuinely-unreachable lines carry annotated v8 ignore. Only the two true bootstraps remain excluded. The AGENTS.md exclusion note is gone.
  • 2c — fixed (fell out of 1d). local:gate green on stock macOS.
  • 2d — fixed. Rebased; every commit carries Signed-off-by.
  • 2e — done. PR description rewritten to match the code (including packaging); skills/ documented in the AGENTS.md and README structure trees with the .claude/skills/ distinction. Agreed on leaving the branch name.
  • 2f — done, with one deviation. Rebased onto current v2/main and re-ran local:gate. Packaging was initially split to a separate branch as you suggested, then folded back after an explicit maintainer decision to ship it in this PR — see §3.
  • 2g — done. Tracking issue chore(mcpi): replace the temporary @inspector/cli source alias with a real shared surface #2461 filed for promoting the @inspector/cli reach-in surface to a shared area; linked from the tsup comment.

§3 Decisions

  • Shipping: maintainer-approved to bundle in this PR, under the new conflict-free bin name mcpdo (your mcpi-collision point drove the rename). Data point: the addition is ~199 KB compressed / ~770 KB unpacked (~16% of the tarball), and the daemon is inert unless the bin is invoked. Without bundling there was no reasonable install story for the experimental client (build-from-source + npm link).
  • Containerizing: agree — no. Your opt-in per-connection isolation recipe (docker run -i --rm --network none …) is now documented in clients/daemon-cli/README.md; a first-class --sandbox flag is deferred as a possible follow-up.

Ready for re-review whenever you are.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The request-size security limit is bypassable, and several correctness and required lint-enforcement issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 4 Medium severity · 4 Low severity

Open (10)
What changed in this PR

Adds mcpdo, an experimental connection-oriented MCP CLI backed by a token-authenticated Unix-socket daemon.

Changes:

  • Implements persistent MCP connections, commands, OAuth, elicitation, streaming, and safe output formatting.
  • Adds extensive tests and integrates the client into build, validation, coverage, and packaging.
  • Documents the new client and ships an agent-facing mcpdo skill.
File Description
AGENTS.md Documents the new client and repository rules.
README.md Adds mcpdo to the project overview.
clients/​cli/​__tests__/​method-types.test.ts Updates shared method-list tests.
clients/​cli/​__tests__/​run-method-mocks.test.ts Updates reusable handler mocks.
clients/​cli/​__tests__/​servers-list.test.ts Tests reusable server-list behavior.
clients/​cli/​src/​cli-oauth-navigation.ts Exposes shared OAuth navigation.
clients/​cli/​src/​cliOAuth.ts Supports reusable OAuth flows.
clients/​cli/​src/​handlers/​consume-outcome.ts Updates shared outcome handling.
clients/​cli/​src/​handlers/​method-types.ts Defines connection-compatible methods.
clients/​cli/​src/​handlers/​run-method.ts Exposes shared MCP method execution.
clients/​cli/​src/​handlers/​servers-list.ts Generalizes server catalog loading.
clients/​cli/​src/​style.ts Exposes CLI styling helpers.
clients/​daemon-cli/​README.md Documents installation and usage.
clients/​daemon-cli/​__tests__/​agent-help.test.ts Tests agent-help output.
clients/​daemon-cli/​__tests__/​authorize.test.ts Tests authorization behavior.
clients/​daemon-cli/​__tests__/​connection-stored-auth.test.ts Tests stored-auth commands.
clients/​daemon-cli/​__tests__/​daemon-connections.test.ts Tests connection lifecycle.
clients/​daemon-cli/​__tests__/​daemon-coverage.test.ts Covers daemon edge cases.
clients/​daemon-cli/​__tests__/​daemon-ipc-glue.test.ts Tests IPC framing and handling.
clients/​daemon-cli/​__tests__/​daemon-paths.test.ts Tests daemon filesystem paths.
clients/​daemon-cli/​__tests__/​daemon-private.test.ts Tests private-daemon authentication.
clients/​daemon-cli/​__tests__/​daemon-stream.test.ts Tests streaming IPC.
clients/​daemon-cli/​__tests__/​dispatch.test.ts Tests RPC and stream dispatch.
clients/​daemon-cli/​__tests__/​elicitation-bridge.test.ts Tests daemon elicitation bridging.
clients/​daemon-cli/​__tests__/​elicitation-client.test.ts Tests elicitation client transport.
clients/​daemon-cli/​__tests__/​elicitation-prompt.test.ts Tests interactive elicitation.
clients/​daemon-cli/​__tests__/​ema-commands.test.ts Tests EMA commands.
clients/​daemon-cli/​__tests__/​ema.test.ts Tests EMA authentication logic.
clients/​daemon-cli/​__tests__/​form-prompt.test.ts Tests form prompting and validation.
clients/​daemon-cli/​__tests__/​form-schema.test.ts Tests elicitation schema parsing.
clients/​daemon-cli/​__tests__/​format-connection.test.ts Tests connection output formatting.
clients/​daemon-cli/​__tests__/​helpers/​mcp-runner.ts Adds daemon CLI test harness.
clients/​daemon-cli/​__tests__/​hoist-connection.test.ts Tests connection argument rewriting.
clients/​daemon-cli/​__tests__/​mcp-auth-coverage.test.ts Covers MCP authentication branches.
clients/​daemon-cli/​__tests__/​mcp-connection.test.ts Tests CLI connection workflows.
clients/​daemon-cli/​__tests__/​mcp-coverage.test.ts Covers command-routing edge cases.
clients/​daemon-cli/​__tests__/​parse-tool-args.test.ts Tests tool argument parsing.
clients/​daemon-cli/​__tests__/​resolve-command.test.ts Tests executable resolution.
clients/​daemon-cli/​__tests__/​sanitize.test.ts Tests terminal sanitization.
clients/​daemon-cli/​eslint.config.js Configures daemon-client linting.
clients/​daemon-cli/​package-lock.json Locks daemon-client dependencies.
clients/​daemon-cli/​package.json Defines scripts and package metadata.
clients/​daemon-cli/​src/​connection/​authorize.ts Implements connect-time OAuth.
clients/​daemon-cli/​src/​connection/​dispatch.ts Dispatches daemon RPCs and streams.
clients/​daemon-cli/​src/​connection/​elicitation-prompt.ts Implements elicitation prompts.
clients/​daemon-cli/​src/​connection/​ema.ts Implements enterprise-managed auth.
clients/​daemon-cli/​src/​connection/​form-prompt.ts Collects form elicitation input.
clients/​daemon-cli/​src/​connection/​form-schema.ts Parses elicitation schemas.
clients/​daemon-cli/​src/​connection/​format-connection.ts Formats command output.
clients/​daemon-cli/​src/​connection/​format-human.ts Provides human-readable formatting.
clients/​daemon-cli/​src/​connection/​mcp.ts Defines the mcpdo command surface.
clients/​daemon-cli/​src/​connection/​parse-tool-args.ts Parses tool-call arguments.
clients/​daemon-cli/​src/​connection/​private-env.ts Creates private-daemon shell exports.
clients/​daemon-cli/​src/​connection/​resolve-command.ts Resolves caller-side executables.
clients/​daemon-cli/​src/​connection/​sanitize.ts Sanitizes terminal-bound data.
clients/​daemon-cli/​src/​connection/​stored-auth.ts Manages persisted authentication.
clients/​daemon-cli/​src/​daemon/​auth.ts Implements daemon token authentication.
clients/​daemon-cli/​src/​daemon/​client.ts Implements request-response IPC.
clients/​daemon-cli/​src/​daemon/​connections.ts Manages persistent MCP connections.
clients/​daemon-cli/​src/​daemon/​elicitation-bridge.ts Bridges elicitation over IPC.
clients/​daemon-cli/​src/​daemon/​ensure.ts Starts and discovers the daemon.
clients/​daemon-cli/​src/​daemon/​framing.ts Encodes and parses IPC frames.
clients/​daemon-cli/​src/​daemon/​index.ts Exports daemon APIs.
clients/​daemon-cli/​src/​daemon/​ipc-glue.ts Accepts and processes socket clients.
clients/​daemon-cli/​src/​daemon/​paths.ts Defines daemon paths and limits.
clients/​daemon-cli/​src/​daemon/​protocol.ts Defines the IPC protocol.
clients/​daemon-cli/​src/​daemon/​run.ts Boots the daemon process.
clients/​daemon-cli/​src/​daemon/​server.ts Implements daemon lifecycle and routing.
clients/​daemon-cli/​src/​daemon/​stream-client.ts Implements streaming IPC clients.
clients/​daemon-cli/​src/​mcp-bin.ts Boots the mcpdo executable.
clients/​daemon-cli/​tsconfig.json Configures source type-checking.
clients/​daemon-cli/​tsconfig.test.json Configures test type-checking.
clients/​daemon-cli/​tsup.config.ts Builds CLI and daemon bundles.
clients/​daemon-cli/​vitest.config.ts Configures tests and coverage.
clients/​tui/​package-lock.json Refreshes the TUI dependency lock.
clients/​web/​package-lock.json Refreshes the web dependency lock.
clients/​web/​src/​test/​core/​auth/​runner-interactive-oauth.test.ts Tests OAuth signal cancellation.
core/​auth/​node/​runner-interactive-oauth.ts Adds optional signal handling.
core/​mcp/​serverList.ts Persists elicitation capability settings.
core/​mcp/​types.ts Defines elicitation capability types.
package.json Wires build, validation, packaging, and bin entry.
scripts/​install-clients.mjs Adds daemon-client installation.
scripts/​lib/​workflow-gate.test.mjs Updates gate coverage assertions.
scripts/​sdk-watch.mjs Includes daemon bundle externals guidance.
scripts/​verify-bundle-externals.mjs Verifies the new multi-entry bundle.
scripts/​verify-format-coverage.mjs Enrolls daemon-client formatting.
scripts/​verify-test-timeouts.mjs Enrolls daemon-client timeouts.
scripts/​verify-test-timeouts.test.mjs Updates timeout guard tests.
skills/​mcpdo/​SKILL.md Adds agent-facing usage guidance.
specification/​v2_catalog_launch_config.md Links the as-built CLI specification.
specification/​v2_cli_tui_launcher.md Documents the additional client surface.
specification/​v2_cli_v2.md Specifies the implemented connection CLI.
Files not reviewed (3)
  • clients/daemon-cli/package-lock.json: Generated file
  • clients/tui/package-lock.json: Generated file
  • clients/web/package-lock.json: Generated file

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread clients/daemon-cli/src/daemon/ensure.ts
Comment thread clients/daemon-cli/src/daemon/ipc-glue.ts Outdated
Comment thread clients/daemon-cli/eslint.config.js
Comment thread clients/daemon-cli/src/connection/elicitation-prompt.ts
Comment thread clients/daemon-cli/src/connection/form-prompt.ts
Comment thread clients/daemon-cli/src/connection/resolve-command.ts
Comment thread AGENTS.md Outdated
Comment thread clients/daemon-cli/package.json Outdated
Comment thread skills/mcpdo/SKILL.md Outdated
Comment thread specification/v2_cli_v2.md Outdated
- ensure: a losing concurrent starter re-reads the winner's published
  daemon.token instead of polling its own dead token into a bogus
  daemon_start_timeout (explicit/private tokens still fail loud); test
- ipc-glue: enforce the 1 MiB line cap per newline-delimited segment so a
  terminated oversized line can't reset the counter past the check, and
  ignore lines after rejection; unit + e2e regression tests
- elicitation: parse the form schema raw and sanitize server-controlled
  strings at render points only, so responses carry the server's own
  keys/values
- form-prompt: reject non-finite numbers ("Infinity" is not a valid JSON
  number)
- resolve-command: honor an empty PATH entry as the current directory
  (POSIX) and return absolute paths for relative entries
- lint: add the type-aware no-floating-promises pass and --max-warnings 0,
  matching clients/cli
- docs: AGENTS.md external-lists brace path mcpdo -> daemon-cli; SKILL.md
  connect example uses --config; spec no longer advertises unregistered
  `initialize`

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unvalidated terminal hyperlinks, an unsafe private-daemon parent directory, and potentially truncated stream output must be corrected before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4 High severity

Open (4)
Resolved since last review (10)
Files not reviewed (3)
  • clients/daemon-cli/package-lock.json: Generated file
  • clients/tui/package-lock.json: Generated file
  • clients/web/package-lock.json: Generated file
Previously missed (3)

In code that hasn't changed since last review

Medium severity Add elicitCapability persistence and fallback coverage

core/​mcp/​serverList.ts:74

The new persisted setting adds valid/invalid read branches and default-omission write behavior, but the existing comprehensive serverList.test.ts suite has no elicitCapability case. Add coverage for all accepted literals, an unknown hand-edited value falling back to the default, and round-trip/default omission so this shared catalog behavior cannot regress unnoticed.

Medium severity Verify the published mcpdo executable and artifacts

package.json:22

This publishes a second executable, but scripts/pack-and-verify.mjs still checks only the installed mcp-inspector bin and web/launcher artifacts. A missing clients/daemon-cli/build, broken bin.mcpdo target, or omitted skills/mcpdo directory would therefore pass the repository's published-tarball verification. Enroll the new files and run the installed mcpdo --help in that check.

Low severity Rename the tracking entry to Connection CLI umbrella

specification/​v2_catalog_launch_config.md:515

The PR explicitly renames the user-facing concept from “session” to “connection,” but this updated row still calls #1432 the “Session CLI umbrella.” Use “Connection CLI umbrella” so the tracking table matches the as-built terminology.

Comment thread clients/daemon-cli/src/connection/dispatch.ts
Comment thread clients/daemon-cli/src/connection/elicitation-prompt.ts Outdated
Comment thread clients/daemon-cli/src/connection/format-human.ts Outdated
Comment thread clients/daemon-cli/src/daemon/paths.ts
- dispatch: chain stream writes and await the chain before returning, so
  mcp-bin's process.exit can't truncate a pending stdout write on piped or
  backpressured output; write errors stay non-fatal as before
- sanitize: isSafeLinkTarget scheme allowlist (https/http) for OSC 8
  hyperlinks; format-human and URL-mode elicitation render every other
  scheme (file:, custom protocol handlers) as plain text
- paths: fail closed unless the predictable $TMPDIR/mcp-conn-<uid> root is
  a real directory owned by the current user, and tighten a loose mode
  fatally instead of best-effort — a shared-/tmp user can no longer plant
  the root (dir or symlink) and keep write control over socket/token paths

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- cli run-method: reject non-finite numbers in tasks/update
  --input-responses — JSON.parse accepts 1e999 as Infinity, which
  serialization for IPC/MCP would silently send as null; recursive
  round-trip validation mirrors the daemon-cli tool-argument parser
- spec: add connections/show to the IPC op list; drop the stale
  "Windows daemon transport" to-do row (named pipes shipped)

Regression test: nested non-finite input-responses value rejected.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Review round 18 — all 3 items fixed in 4c1c96d (no inline threads):

  • cli: tasks/update could silently change overflowing numbers to null — --input-responses '{"a":1e999}' parsed to Infinity, which IPC/MCP serialization would send as null. Parsed input responses are now validated recursively as JSON-round-trippable (mirroring the daemon-cli tool-argument parser from round 17) and rejected with a clear error. Regression test covers a nested non-finite value.
  • spec: IPC op list omitted connections/show — added; the list now matches DaemonOp and the command surface.
  • spec: stale "Windows daemon transport" to-do — removed; named-pipe transport shipped in round 15 (src/daemon/paths.ts + tests).

Full validate, per-file coverage thresholds, and the repo gate are green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The daemon development build script is incompatible with Windows despite the client’s explicit Windows support.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

Comment thread clients/daemon-cli/package.json Outdated
- make build:dev cross-platform: the POSIX-only `;` sequencing and
  /dev/null redirect (invalid under cmd.exe) move into a small
  stop-dev-daemon.mjs pre-build script that best-effort stops a
  resident daemon via execFileSync with stdio ignored — preserving the
  first-build behavior a plain && would break (no build/ yet must not
  skip tsup)

Verified: build:dev succeeds, and the pre-build script exits 0 with no
build present.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson
BobDickinson requested a balanced review from Copilot September 26, 2026 00:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Streaming lacks output backpressure and URI isolation, while ad-hoc EMA is advertised despite lacking required credentials.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Reject or support --ema for ad-hoc targets with missing OAuth settings

clients/​daemon-cli/​src/​connection/​mcp.ts:351

--ema cannot actually work for an ad-hoc target despite advertising that use. The ad-hoc path gets only default settings from loadServerEntries, this CLI exposes no resource-AS client-id/secret flags, and core/auth/ema/emaFlow.ts rejects EMA unless both per-server oauth.clientId and oauth.clientSecret are present. Either reject --ema for ad-hoc targets with actionable guidance, or provide a secure way to supply those required settings.

Medium severity Filter resource updates by URI to prevent subscriber cross-talk

clients/​daemon-cli/​src/​daemon/​server.ts:512

Opening multiple resources/subscribe streams on one named connection produces cross-talk. The shared stream returned here installs a connection-wide resourceUpdated listener (clients/cli/src/handlers/run-method.ts:219-226) but never checks that detail.uri matches the URI requested by this stream, so an update for B is emitted by subscribers for A as well. Filter the event by the subscribed URI before writing it.

Comment thread clients/daemon-cli/src/daemon/stream-client.ts
- Propagate async backpressure through daemon stream output: onData may
  now return a promise, and streamDaemon pauses socket reads until it
  settles, so a fast logging/resource stream can no longer queue
  unbounded pending stdout writes against a slow consumer.
  dispatchConnectionRpc returns its write chain from onData.
- Reject --ema for ad-hoc connect targets up front with actionable
  guidance: EMA requires per-server oauth.clientId/clientSecret that
  only a catalog entry can supply, so the flow could never succeed.
  Help text and withEmaOverride docs updated to match.
- Filter resources/subscribe stream events by the subscribed URI so
  multiple subscribe streams on one named connection no longer
  cross-talk; events without a uri still pass through.
- Cover clients/daemon-cli/scripts with the prettier format globs
  (verify:format-coverage guard flagged round 19's stop-dev-daemon.mjs).
- Tests: backpressure pause/resume, rejected write promise stays
  non-fatal, --ema ad-hoc rejection, URI cross-talk filtering.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Round 20 "previously missed" items are addressed in 87945d7:

  • --ema on ad-hoc targets (clients/daemon-cli/src/connection/mcp.ts): connect now rejects --ema for ad-hoc targets up front with actionable guidance ("add the server to a catalog with oauth.clientId and oauth.clientSecret, then connect by entry name"), rather than failing deep in emaFlow with an opaque error. We deliberately did not add client-id/secret CLI flags — secrets on the command line leak via shell history and process listings, and the catalog entry is the supported secure path. The --ema help text no longer advertises ad-hoc use.
  • resources/subscribe cross-talk (clients/cli/src/handlers/run-method.ts): the stream's resourceUpdated listener now filters events by the subscribed URI, so multiple subscribe streams on one named connection no longer receive each other's updates. Update events carrying no uri (spec-noncompliant server) still pass through, preserving the previous fallback.

Also fixed while gating: the verify:format-coverage guard flagged round 19's clients/daemon-cli/scripts/stop-dev-daemon.mjs as outside the prettier globs; scripts is now included in daemon-cli's format/format:check.

Regression tests added for both findings; full local gate green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Concurrent elicitation and stream ownership can misroute prompts, lose subscriptions, leak streams, and produce unhandled cleanup failures.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (1)
Previously missed (4)

In code that hasn't changed since last review

Medium severity Preserve schema properties named __proto__

clients/​daemon-cli/​src/​connection/​form-prompt.ts:262

A valid schema property named __proto__ is not preserved here: assigning an array-valued multiselect invokes Object.prototype.__proto__ instead of creating an own property, so the accepted payload silently omits the required answer. Build this dictionary with a null prototype, as sanitizeDeep already does for untrusted keys.

Medium severity Clean up subscriptions when the caller disconnects early

clients/​daemon-cli/​src/​daemon/​ipc-glue.ts:185

If the caller disconnects while handle() is awaiting a stream method, this early return abandons the HandleOutcome. resources/subscribe has already subscribed before that outcome is returned, but its cleanup is only created by startStream, so the daemon keeps a hidden subscription with no consumer. Ensure an opened stream is disposed when the socket has already closed, or defer subscription ownership until the producer starts.

Medium severity Handle asynchronous stream unsubscribe failures

clients/​daemon-cli/​src/​daemon/​ipc-glue.ts:196

This try/catch only catches synchronous cleanup failures. The resource stream's stop callback starts unsubscribeFromResource() with void, so after disconnectAll() has already closed clients during daemon shutdown, that promise rejects outside this catch as an unhandled rejection. Make stream cleanup awaitable (or explicitly catch that unsubscribe promise) before treating cleanup errors as ignored.

Medium severity Terminate streams when their named connection closes

clients/​daemon-cli/​src/​daemon/​server.ts:526

The stream has no lifecycle link to its named connection after this starter is returned. If another invocation runs mcpdo disconnect <name> (or the remote transport closes), an existing logging/tail or resource stream remains attached to the stale client and waits until Ctrl-C or daemon idle shutdown instead of terminating. Track active streams by connection and close their sockets/producers when that connection is removed or fails.

Comment thread clients/daemon-cli/src/connection/dispatch.ts Outdated
Comment thread clients/daemon-cli/src/daemon/elicitation-bridge.ts
Comment thread clients/cli/src/handlers/run-method.ts
- Recover the stream output write chain after a rejected write so one
  failed stdout write no longer silently drops every later event.
- Serialize rpc ops per client in the daemon so concurrent RPCs on one
  connection cannot misroute an elicitation prompt to the wrong
  caller's terminal; bridge docs updated.
- Reference-count same-URI resources/subscribe streams: only the first
  consumer subscribes and only the last stream's close unsubscribes,
  so closing one stream no longer silences its same-URI sibling. The
  final unsubscribe's rejection is caught at the source (it can fire
  after daemon disconnectAll during shutdown).
- Preserve form-prompt schema properties named __proto__ by building
  the accepted payload with a null prototype.
- Dispose an opened stream outcome when the caller's socket died while
  the handler ran, so resources/subscribe cannot leak a hidden
  daemon-side subscription with no consumer.
- Tie daemon streams to their connection's lifecycle: a producer-side
  endStream channel plus a statusChange listener ends logging/tail and
  subscribe streams when their named connection disconnects or fails,
  instead of hanging until Ctrl-C or daemon idle shutdown.
- Tests: write-chain recovery, per-connection rpc serialization,
  subscribe refcounting + swallowed unsubscribe rejection, __proto__
  field preservation, mid-handle disposal, producer-side end, stream
  termination on disconnect.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Round 21 "previously missed" items are addressed in f87360f:

  • __proto__ schema property dropped (clients/daemon-cli/src/connection/form-prompt.ts): the accepted payload is now built with a null-prototype object (mirroring sanitizeDeep), so a valid schema property named __proto__ becomes an own property instead of invoking the prototype setter. Test: a required __proto__ string field survives into the accepted content.
  • Subscriptions leak when the caller disconnects mid-handle (clients/daemon-cli/src/daemon/ipc-glue.ts): when the socket is already destroyed after handle() returns, a stream outcome is now started inert and stopped immediately, so resources/subscribe's producer-side subscription is disposed rather than becoming a hidden consumer-less subscription.
  • Async stream-cleanup rejections unhandled (ipc-glue.ts / clients/cli/src/handlers/run-method.ts): the resource stream's stop now catches the unsubscribeFromResource() promise at the source, so a post-disconnectAll rejection during daemon shutdown can no longer surface as an unhandled rejection.
  • Streams outlive their named connection (clients/daemon-cli/src/daemon/server.ts): the stream contract gained a producer-side endStream channel (end frame + socket end), and openStream now wires a statusChange listener that ends the stream when its connection reaches a terminal state — so mcpdo disconnect <name> (or a transport failure) terminates attached logging/tail/subscribe streams instead of leaving them hanging until Ctrl-C or daemon idle shutdown. Test: an open stream ends when its connection is disconnected.

Regression tests added for all items; full local gate green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Connect cancellation, subscription lifecycle, and unnecessary JSON-mode RPC behavior need correction.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid hidden app info RPCs for JSON tool calls

clients/​daemon-cli/​src/​connection/​dispatch.ts:41

Forwarding format: "json" into the shared runMethod has an unintended side effect: tools/call performs collectAppInfo, which may issue an extra resources/read request and wait for its timeout, but mcpdo explicitly discards the returned appInfo. JSON tool calls should not make this hidden RPC; keep formatting in the frontend and omit format from the daemon method arguments.

Comment thread clients/daemon-cli/src/connection/mcp.ts
Comment thread clients/cli/src/handlers/run-method.ts Outdated
- Cancel daemon-side connects when the frontend disconnects: ipc-glue
  aborts a per-socket AbortSignal on close, threaded through handle()
  into ConnectionRegistry.connect, which races client.connect() against
  the abort, tears the in-flight client down, and refuses post-abort
  registration (no pinned pendingConnects, no unwanted late connection).
- Reject explicit resources/unsubscribe while refcounted subscribe
  streams share the URI's subscription, with guidance to close the
  streams instead (prevents silent streams + double unsubscribe).
- Keep `format` frontend-only: dispatch no longer sends it and the
  daemon's stripConnectionFields drops it defensively, so JSON tool
  calls no longer trigger a hidden collectAppInfo resources/read whose
  result mcpdo discards. Explicit --app-info is unaffected.
- Tests: pre-aborted/mid-flight/post-connect cancellation (registry),
  socket-close signal abort (ipc-glue), format stripping end-to-end
  (DaemonServer), rpc params omit format (dispatch), unsubscribe
  rejection + release (run-method).

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Addressed the round-22 "previously missed" item in b7a675b:

Avoid hidden app info RPCs for JSON tool calls (dispatch.ts:41) — format is now a frontend-only concern: dispatchConnectionRpc no longer includes it in the daemon rpc params, and the daemon's stripConnectionFields drops it defensively for any older caller. JSON tool calls therefore no longer trip runMethod's format === "json" app-info branch (the hidden extra resources/read + timeout wait whose result mcpdo discarded). Explicit --app-info still probes via the separate appInfo flag. Tests assert the rpc params omit format (dispatch) and that a daemon tools/call with format: "json" returns no appInfo while appInfo: true still does (end-to-end DaemonServer test).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The change introduces a large security-sensitive daemon and IPC surface, and the authentication documentation still contains a misleading statement.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Correct misleading comment about authentication when the environment variable i…

clients/​daemon-cli/​src/​daemon/​auth.ts:28

This comment contradicts the enforced security model above and in daemon/run.ts: an unset environment variable does not create an unauthenticated daemon; shared mode generates and publishes a required token. Describing it as unauthenticated could lead future callers to weaken or misuse this boundary.

- Correct the misleading getDaemonTokenFromEnv doc comment: an unset
  environment variable selects shared mode, which is still
  authenticated — the daemon generates its own required token and
  publishes it to daemon.token. It never creates an unauthenticated
  daemon.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Addressed the round-23 "previously missed" item in 4012412:

Correct misleading comment about authentication (daemon/auth.ts:28) — the getDaemonTokenFromEnv doc no longer describes shared mode as "unauthenticated". It now states the actual model: an unset environment variable selects shared mode, where the daemon generates its own required token (daemon/run.ts) and publishes it to daemon.token for same-user clients; every daemon requires a token, and the env var only selects who supplies it. Doc-only change, no behavior difference.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Cancellation is lost across the IPC adapter, and concurrent resource subscriptions can corrupt lifecycle accounting.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Handle already-aborted signal before installing abort listener

clients/​daemon-cli/​src/​daemon/​connections.ts:312

There is still an abort race after createConnectionClient(): if the signal becomes aborted before this listener is installed, addEventListener does not replay the event and a stalled client.connect() can remain alive indefinitely. Re-check signal.aborted synchronously before registering the listener; no abort can interleave between that check and addEventListener on this thread.

Comment thread clients/cli/src/handlers/run-method.ts Outdated
Comment thread clients/daemon-cli/src/daemon/server.ts Outdated
Class-level fixes rather than point patches:

- Abort-listener races: new withAbort helper in connections.ts takes a
  thunk, checks aborted synchronously, and installs the listener before
  starting the operation — no check/listen gap can hang a stalled
  connect. Applied the same pre-aborted check to callDaemon and
  streamDaemon, which had the identical class of bug (SIGINT during
  ensureDaemon would never fire their listeners; with timeoutMs 0 the
  request hung forever).
- IPC seam: the acceptDaemonConnection adapter in DaemonServer.start now
  forwards the per-socket abort signal into handleOutcome (it was
  silently dropped, making round-22's cancellation inert over real
  sockets). Added a true end-to-end socket test — client aborts a
  gated connect, daemon must tear down the dial — so this seam cannot
  silently regress again.
- Check-then-act on shared subscription state: resourceStreamRefs
  entries are now { count, ready } reserved synchronously before any
  await; concurrent same-URI subscribes join one in-flight subscribe
  promise, failures roll back their own reservation (last one out
  deletes the entry for clean retry), and stream stops are idempotent
  with stale-generation delete guards.
- Tests: e2e socket cancellation, reconnect-window abort (pre-start
  check), pre-aborted callDaemon/streamDaemon, concurrent subscribe
  sharing + double-stop, subscribe failure rollback + retry.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Addressed the round-24 "previously missed" item in be7d78e:

Handle already-aborted signal before installing abort listener (connections.ts:312) — fixed as a class rather than a spot check: a new withAbort(start, signal, makeError) helper checks signal.aborted synchronously and installs the abort listener before invoking the thunk, so no abort can interleave between check and listen (AbortSignal does not replay). connectLocked uses it for the dial; a pre-aborted signal now means the connect is never even started. The same class of bug existed client-side in callDaemon/streamDaemon (a SIGINT during ensureDaemon pre-aborted their signals before listener install; with timeoutMs: 0 the request hung forever) — both now check first. Tests: abort landing in the reconnect-teardown window (dial never starts), pre-aborted callDaemon rejects immediately, pre-aborted streamDaemon finishes immediately.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Stream setup can fail prematurely and required multiselect forms cannot submit a valid empty selection.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Stream setup ignores configured timeouts and may leak subscriptions

clients/​daemon-cli/​src/​connection/​dispatch.ts:64

Stream setup is still subject to streamDaemon's default 60-second client deadline. openStream() awaits the underlying MCP operation before acknowledging the stream, so a configured resources/subscribe request timeout above 60 seconds (or an unlimited one) can remain valid daemon-side while this command reports daemon_timeout; unlike the RPC path below, it may also leave the eventual subscription running after the caller exits. Disable the fixed IPC deadline here, and update streamDaemon so timeoutMs: 0 actually disables its timer as documented by DaemonClientOptions.

Medium severity Required empty multiselect causes infinite loop

clients/​daemon-cli/​src/​connection/​form-prompt.ts:107

A required property may still legally be an empty array. For a multiselect with no minItems (or minItems: 0), blank input should therefore submit []; this branch instead loops forever, so users cannot produce a schema-valid response. Preserve omission for optional fields, but accept an empty array for required multiselects whose minimum permits it.

- dispatch: stream path now passes timeoutMs 0 to streamDaemon, matching
  the rpc path — core enforces the configured MCP request timeout
  daemon-side, so a fixed local 60s deadline falsely failed valid
  long-running stream setups.
- stream-client: honor timeoutMs 0 as "no deadline" (mirrors callDaemon);
  the previous unconditional setTimeout would have fired a 0ms timer
  immediately instead of disabling it.
- form-prompt: blank input on a required multiselect now submits [] when
  (minItems ?? 0) === 0 — JSON Schema "required" only demands presence,
  and the prompt previously looped forever with no way to select none;
  minItems >= 1 still re-prompts with the minimum.
- tests: streamDaemon timeoutMs-0 deadline-disable regression, dispatch
  stream timeoutMs assertion, required-multiselect blank submit and
  minItems re-prompt cases.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author

Round 25 fixes (commit a88e4bd) — both "previously missed" items from review 5324826242:

  1. Stream setup ignored configured timeouts (dispatch.ts / stream-client.ts): the stream path now passes timeoutMs: 0 to streamDaemon, matching the rpc path — core enforces the configured MCP request timeout daemon-side, so the fixed local 60s deadline could falsely fail valid long-running stream setups. Also fixed the underlying class issue: streamDaemon's open-deadline timer was an unconditional setTimeout, so timeoutMs: 0 (documented as "no deadline") would have fired immediately; it now mirrors callDaemon and skips the timer when timeoutMs <= 0. (The subscription-leak half is already covered by the earlier mid-handle stream disposal, which unsubscribes when the socket dies before the outcome.) Regression tests: timeoutMs-0 disables the deadline against a slow-responding socket; dispatch asserts timeoutMs: 0 reaches streamDaemon.

  2. Required multiselect infinite loop on blank input (form-prompt.ts): JSON Schema required only demands the key be present — [] is a valid value unless minItems forbids it. Blank input on a required multiselect now submits [] when (minItems ?? 0) === 0; when minItems >= 1 it re-prompts with "Select at least N" instead of the misleading "This field is required". Optional fields still omit the key. Regression tests cover both branches.

Full gate green (daemon-cli 322 tests, coverage clean).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A disconnect race can leave a newly opened stream hanging indefinitely after its terminal status event is missed.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread clients/daemon-cli/src/daemon/server.ts
- server: close the stream disconnect event-registration race. The
  statusChange listener was only installed inside startStream, which
  ipc-glue invokes after the ok frame; a disconnect completing between
  runMethod() and that install lost the terminal event, leaving the
  stream open against a dead client until Ctrl-C or idle shutdown.
  startStream now installs the listener first and then checks the
  current status synchronously — terminal status is persistent state,
  so the check closes every window back to when the stream went live.
- test: open a stream, disconnect in the runMethod->startStream window,
  assert startStream ends the stream immediately.

Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Client disconnects do not cancel daemon-side RPC or stream setup, potentially leaving abandoned work blocking the connection.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@BobDickinson

Copy link
Copy Markdown
Contributor Author

Note on the round-27 review headline (review 5324968883, commit 10db834 — Findings: None):

The overview sentence — "Client disconnects do not cancel daemon-side RPC or stream setup, potentially leaving abandoned work blocking the connection" — had no finding filed with it. For the record, it describes a real but intentional, bounded design property rather than a defect:

  • When a caller aborts (Ctrl-C closes the client socket), the per-socket AbortSignal (ipc-glue.ts) is forwarded into handle(), but only connect setup honors it (withAbort in connections.ts). A mid-flight rpc op (e.g. a slow tool call) runs to completion daemon-side.
  • Because rpc ops are serialized per connection (rpcQueues, required for exact elicitation routing), a subsequent command on that connection can briefly queue behind the abandoned op. This is bounded by the daemon-side MCP request timeout; nothing leaks or hangs indefinitely — streams are torn down on socket close, and terminal connection status ends them (see 10db834).
  • Honoring the signal mid-rpc would require threading cancellation through runMethod into core's InspectorClient request path (issuing MCP cancelled notifications) — a core-level change out of scope for this PR. Left as a possible follow-up if head-of-line blocking after abandoned long ops proves to matter in practice.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inspector mcpi client

3 participants